-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Adds BS5 modal components #234
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #234 +/- ##
==========================================
- Coverage 86.93% 82.96% -3.97%
==========================================
Files 120 125 +5
Lines 3107 3252 +145
Branches 330 337 +7
==========================================
- Hits 2701 2698 -3
- Misses 304 452 +148
Partials 102 102 ☔ View full report in Codecov by Sentry. |
So coool! 😎 I will look into it! Can you get the pre-commit stuff sorted out meanwhile? |
Hi @fsbraun, it is my first pull request ever. Maybe you have to look over the code twice. 😇 |
Hey @mavoIn ! Can I ask you to do the following things:
I hopefully will be able to test the modal in a pet project tomorrow. |
HI @fsbraun , |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I need to better understand how to use the modal? Maybe you can give an example in the docs?
Also, can you take a look at the import issue showing up in tests? And the migrations seem not up-to-date. You may update the initial migration - they are DB no-ops anyways. |
Hello @fsbraun, thanks a lot for all your advices. There are problems importing the tests because I was only able to insert a code framework from the collapse example. Please let me know so that you can fix the tests or I might have to ask friends for further support. Best mavoIn |
Co-authored-by: Fabian Braun <[email protected]>
@mavoIn I see you have updated quite a few things! Thank you. Still, the tests are failing. Try running them locally: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some concerns about the structure:
- Modal triggers should be possible anywhere on a page
- The Modal plugin is not needed as far as I can see. I think the
<div class="modal"></div>
can go into the modal container. - I still would need to understand a use case with a static trigger. Can you give an exmaple?
djangocms_frontend/contrib/modal/templates/djangocms_frontend/bootstrap5/modal-inner.html
Show resolved
Hide resolved
Co-authored-by: Fabian Braun <[email protected]>
Co-authored-by: Fabian Braun <[email protected]>
Change parent_classes CardPlugin to ModalPlugin
Hi @fsbraun, what do you exactly mean with static trigger? |
As discussed in #233 I added a modal component. It works like the cards component. Basic documentation added as well.